Codecov Report

Attention: Patch coverage is 94.11765% with 1 line in your changes missing coverage. Please review.

Project coverage is 90.30%. Comparing base (dfb2dff) to head (78be697).
Report is 167 commits behind head on main.

@@            Coverage Diff             @@
##             main    #2174      +/-   ##
==========================================
- Coverage   90.97%   90.30%   -0.68%     
==========================================
  Files         146      147       +1     
  Lines        7492     7260     -232     
  Branches     1502     1509       +7     
==========================================
- Hits         6816     6556     -260     
- Misses        676      704      +28     

I see that adding this env var was already commented in the linked PR here. I guess you already have the use case for this :)

I do keep getting the request for the simplest possible setup, and iterating from there. So this is similar to how many folks have gotten started with the auto-instrumentations-node package as it is. Start with everything enabled by default, and then disable one by one as you decide what you don't want, instead of trying to figure out everything you do want. Later as you understand your telemetry more it may be possible to switch from one to the other (start with disabling specific ones, then later only need to enable specific ones). But yes, that is the reason for this. Also other languages have options for both disable and enable specific instrumentations, including dotnet and java, and Python has Disabled instead of Enabled.

@open-telemetry/javascript-approvers Just wanted to ping to see if anyone else wanted a review before merging, since this is the auto-instr package so it is a bit more far-reaching than a single instrumentation. Otherwise will merge on Monday!

@open-telemetry/javascript-approvers Just wanted to ping to see if anyone else wanted a review before merging, since this is the auto-instr package so it is a bit more far-reaching than a single instrumentation. Otherwise will merge on Monday!

I'm neutral on that feature. I'm usually a big proponent of people knowing which instrumentations are enabled, that's why I prefer explicitly enabling them - if we have enough that asked for this after we added the enabled env var then adding this should be okay. :)

@open-telemetry/javascript-approvers Just wanted to ping to see if anyone else wanted a review before merging, since this is the auto-instr package so it is a bit more far-reaching than a single instrumentation. Otherwise will merge on Monday!

I'm neutral on that feature. I'm usually a big proponent of people knowing which instrumentations are enabled, that's why I prefer explicitly enabling them - if we have enough that asked for this after we added the enabled env var then adding this should be okay. :)

This is essential for an out of the box setup using the Kubernetes instrumentation CRD to auto-instrument existing services. We switched from using the datadog-agent to the opentelemetry-collector and the fs instrumentation overran our budget. I think it also adds parity to the other instrumentation libraries, e.g.

OTEL_DOTNET_AUTO_METRICS_{0}_INSTRUMENTATION_ENABLED

Most of the guides you'll find for the NodeSdk look like this:

const sdk: NodeSDK = new NodeSdk({
  instrumentations: [    
    getNodeAutoInstrumentations({
      // We recommend disabling fs automatic instrumentation because 
      // it can be noisy and expensive during startup
      '@opentelemetry/instrumentation-fs': {
        enabled: false,
      },
    }),
  ],
});

By adding this environment variable, you can replicate this configuration using the opentelemetry-collector's injector without modifying existing services.

Thanks for pushing through this change Jamie - where I work we were doing a lot of OpenTelemetry research earlier in the year and the whole "starting up an otel-instrumented hello world nodejs express app spits out 600 filesystem traces from module loading" thing was a big early problem I came across. I luckily also came across your example app repo that had the magic requireParentSpan: true line which stopped this, but having an environment variable now for the zero-code instrumentation case will make adopting OTel more attractive to any of our JS teams :-). (I happened to be checking back in on issue #1344 this week for one of said teams and found my way to this PR and felt compelled to say thanks!)